Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor flashes [WIP - Questions] #455

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

patriciomacadden
Copy link
Contributor

The code that displays flashes is repeated a few times in the scaffold and authentication generators.

When you run the generators, you normally end up extracting those lines into a separate partial. This PR aims to simplify that process by creating the app/views/application/_flashes.html.erb partial that is shared for the entire app when running rails g authentication or rails g scaffold .... Once you have it, you can customize it to your needs and the next time the generators are run, you'll be prompted to leave it or overwrite it, etc.

An alternative to this (creating the file when running the generators) would be to create the partial when running rails tailwindcss:install and insert it right into the layout. tbh I like this approach better, but if we go that way we have a few issues:

  1. the tailwindcss:install task adds the flex class to the layout's main container, so we'd have to remove it. Not sure why it's added in the first place but it might be a problem.
  2. the session and passwords views have the w-2/3 class, meaning the flash would be left-aligned while the session/password form would be centered. we'd end up adding a condition to the flash like "make it w-2/3 and mx-auto if it's the session or passwords controller", which is far from ideal.

What do you think?

The whole point of this PR and my previous ones are to make the scaffold generator as "ready to use" as possible without many modifications. Hope it makes sense 🙂

@flavorjones
Copy link
Member

Just a couple of thoughts:

In this and a few other PRs, the class values have been re-ordered (I imagine with some kind of editor plugin like prettier). This makes the PRs very noisy and hard to read, so it's harder to give feedback on what's meaningfully changed. I'm generally in favor of ordering them sensibly, but can you either avoid committing those changes to PRs, or else just give us one big PR with all the class ordering changes in it?

I'm not sure why flex is added to the primary layout. If you think it should be removed to make other things easier, I'm open to it, but we need to document it and maybe think about how to make it easy for people to add if they think it's helpful.

I think I like the idea of having a _flashes partial, and making it part of the site layout. I think it should be rendered with a consistent style regardless of how the page content is rendered, though. Why not just center the flash content? Maybe I'm misunderstanding the issue with flash alignment and content width, and if so I apologize.

@patriciomacadden
Copy link
Contributor Author

patriciomacadden commented Jan 20, 2025

Completely agree on the class ordering thing - I've turned it off and removed all the reorderings. I think i mentioned in another PR but I'm using a vscode plugin and I'm not expecting anyone else to use it, just doesn't make sense. So turning it off is the sensible thing to do.

I'm not sure why flex was added in the primary layout either and I'm not sure what it tries to accomplish. but it makes new views weird. For example, if I add 2 small divs to the index view:

<div>#1</div>
<div>#2</div>

<div class="w-full">
<!-- ... and the rest of the view -->

I'll get this:

image

which is weird if I'm expecting to get something like this:

image

To obtain that behavior, I had to wrap everything in another div:

<div class="w-full">
  <div>#1</div>
  <div>#2</div>

  <div class="w-full">
  <!-- ... and the rest of the view -->

Which can be avoided if we don't use the flex.

If we render the flashes in the layout (which is why I prefer too), then since we have the flex class we'll get something like:

image

if we remove the flex class, then it looks misaligned, because the sign in form (and new/edit password) are mx-auto w-2/3.

image

I can center the flash, I but didn't want to change its design drastically.

So, by keeping it at the view level instead of layout level, we keep the flash left-aligned to parent container.

So I'm not sure. In one hand, I'd prefer to generate the flashes partial when installing and adding it to the layout, but on the other hand I don't want to mess up the design or remove the flex class (although I'm not 100% convinced we need it - need to investigate).

What do you think?

@patriciomacadden
Copy link
Contributor Author

this is the PR that added the flex class: https://github.com/rails/tailwindcss-rails/pull/71/files#diff-82d31adf98ad40bfa67ed05464d36790777703adead66a2478d97320061f136c

There's no explanation why it should be a flex container though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants